Pull request overview

Adjusts ZstandardCompressedContent to avoid producing non-RFC-compliant "zstd" request bodies (RFC 9659 window requirement) and updates functional tests to validate round-tripping across compression levels under an 8 MiB (2^23) max window decoder.

Changes:

• Clamp CompressionLevel.SmallestSize for zstd request compression by switching to explicit ZstandardCompressionOptions with WindowLog2 = 23.
• Expand Brotli/Zstandard functional tests to cover all CompressionLevel values and validate zstd output is decodable with MaxWindowLog2 = 23.
• Update test payload to better exercise compression behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Holistic Review

Motivation: RFC 9659 requires that the zstd HTTP content coding be decodable with a window size of at least 8 MB (2^23) and recommends encoders not exceed it. Previously ZstandardCompressedContent mapped CompressionLevel.SmallestSize to zstd's maximum quality, which can select a window larger than 8 MB, producing frames a conformant server may reject. This PR is a follow-up to #130082 to close that gap.

Approach: For the CompressionLevel constructor, SmallestSize is special-cased to use an explicit ZstandardCompressionOptions with Quality = MaxQuality and WindowLog2 = 23, routing it through the options-based stream path instead of the level-based path. Other levels are unchanged. The round-trip test is expanded to a [Theory] covering all four compression levels for both br and zstd, uses a larger, more compressible payload to exercise larger windows, and configures the test decoder with MaxWindowLog2 = 23 to mirror an RFC-conformant server.

Summary: The change is small, targeted, and correct. Capping the window at 2^23 for SmallestSize while preserving max quality is the right way to stay RFC-compliant without regressing other levels, and the decoder-side test change validates decodability within the RFC window bound. Test coverage is meaningfully improved by parameterizing over all levels. I found one minor naming-convention issue (inline). The PR author's open question about whether to also validate/reject non-compliant user-provided ZstandardCompressionOptions is worth resolving, but it is a deliberate scoping decision rather than a defect in this patch. Verdict: LGTM aside from the minor style nit.

Detailed Findings

• Naming convention (minor, inline): The new static field s_SmallestSizeRfcOptions uses PascalCase after the s_ prefix; repo convention is s_camelCase (e.g. s_smallestSizeRfcOptions).

Non-blocking observations

• The open question raised in the PR description (whether to prevent users from constructing non-RFC-compliant content via the ZstandardCompressionOptions constructor overload) is out of scope for this patch. This PR only constrains the CompressionLevel-based path; the options-based constructor still lets callers specify any WindowLog2 up to MaxWindowLog2 (2^31). That is a reasonable choice (explicit options imply the caller knows what they want), but consider documenting the RFC recommendation on the options constructor.
• The test's larger, repetitive payload (Enumerable.Repeat(...)) is a good way to encourage the encoder toward a larger window, strengthening the regression value of the SmallestSize case.

Generated by Holistic Review · 61.8 AIC · ⌖ 10 AIC · ⊞ 10K

Open Question: What do we for user-provided ZstandardCompressionOptions? should we prevent users from creating non-compliant content?

It wouldn't hurt to mention in remarks for the ZstandardCompressionOptions ctor overload that they may want to limit the window.

Given that options like smallest size are likely unpractically slow in practice anyway, I think it's fine to allow the user to set non-compliant options.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

            if (compressionLevel == CompressionLevel.SmallestSize)
            {
                // use RFC-compliant options for SmallestSize to avoid producing frames that a conformant server would reject
                _compressionOptions = s_smallestSizeRfcOptions;
            }

        // RFC 9659 requires zstd decoders for the "zstd" content coding to support a window size of at
        // least 8 MB (2^23) and recommends that encoders not produce frames requiring a larger window.
        // Some compression levels (notably CompressionLevel.SmallestSize) would otherwise select a larger
        // window, producing payloads that a conformant server would reject. See RFC 9659, Section 3.
        private const int RfcMaxWindowLog2 = 23;

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

            if (compressionLevel == CompressionLevel.SmallestSize)
            {
                // use RFC-compliant options for SmallestSize to avoid producing frames that a conformant server would reject
                _compressionOptions = s_smallestSizeRfcOptions;
            }
            else
            {
                _compressionLevel = compressionLevel;
            }

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

            if (compressionLevel == CompressionLevel.SmallestSize)
            {
                // use RFC-compliant options for SmallestSize to avoid producing frames that a conformant server would reject
                _compressionOptions = s_smallestSizeRfcOptions;
            }
            else
            {
                _compressionLevel = compressionLevel;
            }

        /// <remarks>
        /// RFC 9659 requires that the "zstd" content coding be decodable with a window size of 8 MB (2^23) and
        /// recommends that encoders not produce frames requiring a larger window. When supplying custom options,
        /// consider limiting <see cref="ZstandardCompressionOptions.WindowLog2"/> to 23 or less so that the
        /// produced content is accepted by servers that enforce this limit.
        /// </remarks>
        public ZstandardCompressedContent(HttpContent content, ZstandardCompressionOptions compressionOptions)

Holistic Review

Motivation: RFC 9659 requires that the zstd HTTP content coding be decodable with an 8 MB (2^23) window and recommends encoders not exceed it. ZstandardCompressedContent mapped CompressionLevel.SmallestSize to zstd's maximum quality, which can select a larger window, producing frames a conformant server may reject. This PR (follow-up to #130082) closes that gap.

Approach: The CompressionLevel constructor special-cases SmallestSize to explicit ZstandardCompressionOptions with Quality = MaxQuality and WindowLog2 = 23, routing it through the options path; other levels are unchanged. The round-trip test is parameterized across all four levels for br and zstd, uses a larger, more compressible payload, configures the decoder with MaxWindowLog2 = 23 to mirror an RFC-conformant server, and skips zstd+SmallestSize on 32-bit processes to avoid intermittent OOM. The single incremental commit since the prior review adds a <remarks> block to the CompressionLevel constructor documenting the RFC window behavior for SmallestSize — mirroring the <remarks> already present on the options constructor.

Summary: The incremental change is documentation-only and correct: the new <remarks> accurately describes the SmallestSize window-limiting behavior implemented in the constructor and is consistent with the class-level comment and the options-constructor remarks. It introduces no behavioral change and no risk. The cumulative change remains small, targeted, and RFC-compliant without regressing other levels. No new actionable findings. Verdict: LGTM.

Assessment History

• review 4730926033 (commit 70e4987): verdict LGTM with a minor style nit. Current verdict: LGTM. Assessment is unchanged in motivation, approach, and risk; the flagged naming nit and documentation suggestion were subsequently resolved.
• review 4733542998 (commit 9db9336): verdict LGTM. Current verdict: LGTM. Assessment is unchanged; the only new commit (f8bf24f) adds an XML-doc <remarks> block to the CompressionLevel constructor, a purely documentary refinement with no effect on motivation, approach, or risk.

Detailed Findings

None. The only incremental change adds accurate XML documentation to the CompressionLevel constructor.

Non-blocking observations

• The new <remarks> on the CompressionLevel constructor now parallels the existing <remarks> on the ZstandardCompressionOptions constructor, giving consistent RFC guidance across both entry points.
• The previously noted open question about validating user-supplied non-RFC ZstandardCompressionOptions remains out of scope; the documentation approach continues to be a reasonable middle ground.

Generated by Holistic Review · 58 AIC · ⌖ 16.1 AIC · ⊞ 10K